-
-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix actix match name bug #539
fix actix match name bug #539
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE to self: Axum recently got something similar which we might want to support with a feature flag as well:
https://docs.rs/axum/latest/axum/extract/struct.MatchedPath.html
sentry-actix/src/lib.rs
Outdated
}; | ||
/// Extract a transaction name from the HTTP request | ||
fn transaction_name_from_http(req: &ServiceRequest) -> String { | ||
format!("{} {}", req.method(), req.uri()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe req.uri()
might have too high of a cardinality and might even contain PII.
I believe match_pattern
is the thing we want, is it documented to give you /user/{id}/profile
for example: https://docs.rs/actix-web/latest/actix_web/struct.HttpRequest.html#method.match_pattern
match_name
seems to refer to the function used for logging? Not sure, there is no example.
Adding a req.method()
prefix does seem like a very good thing indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree 100%.
I've changed the code to use request.match_pattern()
, fixed the broken tests and added an extra one that does a POST with a pattern.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
==========================================
+ Coverage 70.73% 70.87% +0.14%
==========================================
Files 66 66
Lines 6601 6626 +25
==========================================
+ Hits 4669 4696 +27
+ Misses 1932 1930 -2 |
Issue
Actix
request.match_name()
is buggy.It doesn't care about the HTTP verb. It only looks at the
path
. Thus 2 routes with the samepath
will have the same match name.Proposal
Previously,
format!("{} {}", req.method(), req.uri())
was already used as a fallback for the transaction name.These changes make it as the default transaction name.